Skip to content

fix(firefox): handle null args in FirefoxOptions.merge() (#15991) #16043

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 2 commits into
base: trunk
Choose a base branch
from
Open

fix(firefox): handle null args in FirefoxOptions.merge() (#15991) #16043

wants to merge 2 commits into from
+4 −2

Conversation

zidanmp
Copy link

@zidanmp zidanmp commented Jul 12, 2025 •
edited by qodo-merge-pro bot
Loading

User description

🔗 Related Issues

Fixes #15991

💥 What does this PR do?

Adds a null check to prevent a NullPointerException in FirefoxOptions.merge() when merging user-defined moz:firefoxOptions.
The issue occurred because .contains(...) was called on a null list when "args" was present but not properly initialized. This fix ensures the list is not null before using it.

🔧 Implementation Notes

Used a simple null check before iterating over arguments:

if (arguments != null) {
...
}

PR Type

Bug fix

Description

  • Fix NullPointerException in FirefoxOptions.merge() method

  • Add null checks before accessing existing arguments list

  • Prevent crashes when merging user-defined moz:firefoxOptions

Changes diagram

flowchart LR
  A["FirefoxOptions.merge()"] --> B["Check existing args"]
  B --> C["Add null check"]
  C --> D["Safe argument merging"]
Loading

Changes walkthrough 📝

Relevant files
Bug fix
FirefoxOptions.java
Add null safety to argument merging                                           

java/src/org/openqa/selenium/firefox/FirefoxOptions.java

  • Added null checks before accessing existing arguments list
  • Extracted list reference to variable for cleaner code
  • Applied fix to two locations in merge() method
    		• +4/-2     
    Need help?
  • Type /help how to ... in the comments thread for any questions about Qodo Merge usage.
  • Check out the documentation for more information.
    •

    @CLAassistant
    Copy link

    CLAassistant commented Jul 12, 2025 •
    edited
    Loading

    CLA assistant check
    All committers have signed the CLA.

    @selenium-ci selenium-ci added the C-java Java Bindings label Jul 12, 2025
    @qodo-merge-pro Qodo Merge Pro
    Copy link
    Contributor

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Code Duplication

    The same null check and variable extraction pattern is duplicated in two locations. Consider extracting this logic into a helper method to improve maintainability and reduce code duplication.

     
      List<String> existingArgs = (List<String>) newInstance.firefoxOptions.get(Keys.ARGS.key());
  if (existingArgs == null || !existingArgs.contains(arg)) {
    newInstance.addArguments(arg);
  }
});
    Performance Impact

    The firefoxOptions.get(Keys.ARGS.key()) call is now executed for every argument in the forEach loop instead of once before the loop. This could impact performance when processing many arguments.

     
              List<String> existingArgs = (List<String>) newInstance.firefoxOptions.get(Keys.ARGS.key());
          if (existingArgs == null || !existingArgs.contains(arg)) {
            newInstance.addArguments(arg);
          }
        });
  }

  if (name.equals(Keys.PREFS.key) && capabilities.getCapability(name) != null) {
    Map<String, Object> prefs = (Map<String, Object>) (capabilities.getCapability(("prefs")));
    prefs.forEach(newInstance::addPreference);
  }

  if (name.equals(Keys.PROFILE.key) && capabilities.getCapability(name) != null) {
    String rawProfile = (String) capabilities.getCapability("profile");
    try {
      newInstance.setProfile(FirefoxProfile.fromJson(rawProfile));
    } catch (IOException e) {
      throw new WebDriverException(e);
    }
  }

  if (name.equals(Keys.BINARY.key) && capabilities.getCapability(name) != null) {
    Object binary = capabilities.getCapability("binary");
    if (binary instanceof String) {
      newInstance.setBinary((String) binary);
    } else if (binary instanceof Path) {
      newInstance.setBinary((Path) binary);
    }
  }

  if (name.equals(Keys.LOG.key) && capabilities.getCapability(name) != null) {
    Map<String, Object> logLevelMap = (Map<String, Object>) capabilities.getCapability("log");
    FirefoxDriverLogLevel logLevel =
        FirefoxDriverLogLevel.fromString((String) logLevelMap.get("level"));
    if (logLevel != null) {
      newInstance.setLogLevel(logLevel);
    }
  }
}

if (capabilities instanceof FirefoxOptions) {
  newInstance.mirror((FirefoxOptions) capabilities);
} else {
  Object optionsValue = capabilities.getCapability(FIREFOX_OPTIONS);

  if (optionsValue instanceof Map) {
    @SuppressWarnings("unchecked")
    Map<String, Object> options = (Map<String, Object>) optionsValue;

    @SuppressWarnings("unchecked")
    List<String> arguments = (List<String>) (options.getOrDefault("args", new ArrayList<>()));
    @SuppressWarnings("unchecked")
    Map<String, Object> prefs =
        (Map<String, Object>) options.getOrDefault("prefs", new HashMap<>());
    String rawProfile = (String) options.get("profile");
    @SuppressWarnings("unchecked")
    Map<String, Object> logLevelMap =
        (Map<String, Object>) options.getOrDefault("log", new HashMap<>());
    FirefoxDriverLogLevel logLevel =
        FirefoxDriverLogLevel.fromString((String) logLevelMap.get("level"));

    arguments.forEach(
        arg -> {
          List<String> existingArgs = (List<String>) newInstance.firefoxOptions.get(Keys.ARGS.key());
          if (existingArgs == null || !existingArgs.contains(arg)) {
            newInstance.addArguments(arg);
          }

    @qodo-merge-pro Qodo Merge Pro
    Copy link
    Contributor

    qodo-merge-pro bot commented Jul 12, 2025 •
    edited
    Loading

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    General
    Add type safety check

    The unchecked cast to List could fail if the stored value is not a List or
    contains non-String elements. Add a type check before casting to prevent
    ClassCastException.

    java/src/org/openqa/selenium/firefox/FirefoxOptions.java [308-311]

    -List<String> existingArgs = (List<String>) newInstance.firefoxOptions.get(Keys.ARGS.key());
+Object argsObj = newInstance.firefoxOptions.get(Keys.ARGS.key());
+List<String> existingArgs = (argsObj instanceof List) ? (List<String>) argsObj : null;
 if (existingArgs == null || !existingArgs.contains(arg)) {
   newInstance.addArguments(arg);
 }
    • Apply / Chat
    Suggestion importance[1-10]: 7

    __

    Why: The suggestion correctly points out that the unchecked cast could lead to a ClassCastException and proposes using an instanceof check to improve type safety and make the code more robust.

    		Medium
    • Update

    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Reviewers
    No reviews
    Assignees
    No one assigned
    Labels
    C-java Java Bindings Review effort 2/5
    Projects
    None yet
    Milestone
    No milestone
    Development

    Successfully merging this pull request may close these issues.

    [🐛 Bug]: FirefoxOptions.merge() throws NullPointerException when merging user-defined "moz:firefoxOptions" map
    4 participants
    @zidanmp @CLAassistant @selenium-ci @navin772